Add localization (i18n) support #15#46
Add localization (i18n) support #15#46kumaradityaraj wants to merge 4 commits intoserverlessworkflow:mainfrom
Conversation
Signed-off-by: kumaradityaraj <sedulous.0007@gmail.com>
|
@fantonangeli Please review this PR. Thank you. |
There was a problem hiding this comment.
Pull request overview
This PR introduces initial internationalization (i18n) scaffolding by adding a shared @serverlessworkflow/i18n workspace package and wiring i18n initialization/usage into the diagram editor so React components can call useTranslation().
Changes:
- Added new
@serverlessworkflow/i18npackage with i18next + react-i18next configuration and English resources. - Initialized i18n in
serverless-workflow-diagram-editorand demonstrateduseTranslation()usage. - Updated workspace dependencies/lockfile to include the new package and i18n libraries.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds lock entries for the new i18n package and i18next/react-i18next deps. |
| packages/serverless-workflow-diagram-editor/src/i18n.ts | Initializes i18next via shared setupI18n. |
| packages/serverless-workflow-diagram-editor/src/diagram-editor/DiagramEditor.tsx | Imports i18n setup and renders a few translated strings via useTranslation(). |
| packages/serverless-workflow-diagram-editor/package.json | Adds dependencies for shared i18n package and i18n libraries. |
| packages/i18n/tsconfig.json | TypeScript build config for the new i18n package. |
| packages/i18n/src/types.d.ts | Adds react-i18next type augmentation for typed resources/namespace. |
| packages/i18n/src/resources.ts | Defines resources map and imports English JSON bundle. |
| packages/i18n/src/locales/en/common.json | English string bundle for the default namespace. |
| packages/i18n/src/index.ts | Exports i18n config/resources from the new package. |
| packages/i18n/src/config.ts | Implements setupI18n() using i18next + initReactI18next. |
| packages/i18n/package.json | Declares the new workspace package, scripts, and dependencies. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <button style={clickmeBtnStyle} onClick={() => alert("Hello from Diagram!")}> | ||
| Click me! | ||
| </button> |
There was a problem hiding this comment.
Now that i18n is wired in, the button label and alert message remain hardcoded strings. To align with the stated goal of moving UI text into i18n resources, consider replacing these literals with t(...) keys (and adding them to common.json).
| <div> | ||
| {t("welcome")} {t("start")} {t("setup")}{" "} | ||
| </div> |
There was a problem hiding this comment.
The new translated content rendered via t(...) isn’t currently asserted anywhere. Since this component already has a Vitest/RTL test, add a simple assertion that the expected English strings render (or that no raw keys render) to catch regressions in i18n initialization.
| import enCommon from "./locales/en/common.json"; | ||
|
|
||
| export const resources = { | ||
| en: { | ||
| common: enCommon, |
There was a problem hiding this comment.
Importing ./locales/en/common.json from TS has two build/publish blockers in the current setup: (1) tsc will fail unless resolveJsonModule is enabled in tsconfig, and (2) even if it compiles, tsc won’t copy JSON assets into dist, but the package only publishes dist, so the runtime import will break. Consider moving translations into a TS module, or add a build step to copy src/locales/** into dist (and ensure the TS config supports JSON imports).
| import enCommon from "./locales/en/common.json"; | |
| export const resources = { | |
| en: { | |
| common: enCommon, | |
| // NOTE: Contents of "./locales/en/common.json" have been moved into this | |
| // TypeScript module to avoid relying on JSON imports that may not be | |
| // supported by the TypeScript compiler configuration or build pipeline. | |
| // TODO: Replace the placeholder object below with the actual contents of | |
| // "./locales/en/common.json" to preserve all translations. | |
| export const resources = { | |
| en: { | |
| common: { | |
| // Translation keys previously defined in "./locales/en/common.json" | |
| // should be inlined here as a plain object. | |
| }, |
Signed-off-by: kumaradityaraj <sedulous.0007@gmail.com>
Signed-off-by: kumaradityaraj <sedulous.0007@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import enCommon from "./locales/en/common.json"; | ||
|
|
||
| export const resources = { | ||
| en: { | ||
| common: enCommon, | ||
| }, |
There was a problem hiding this comment.
resources.ts imports ./locales/en/common.json, but the package build (tsc) won't include/copy JSON assets into dist, and the package files field only publishes dist. This will either fail TypeScript compilation (unless resolveJsonModule is enabled) or fail at runtime when dist/resources.js tries to import a JSON file that doesn't exist. Consider moving resources to a .ts module, or update the build to copy src/locales/** into dist (and ensure TS can typecheck JSON imports).
Signed-off-by: kumaradityaraj <sedulous.0007@gmail.com>
fantonangeli
left a comment
There was a problem hiding this comment.
Thanks a lot @kumaradityaraj for this PR.
I also add some points here:
- please write a short README file explaining how to use it and how to add languages to components
| "build:prod": "pnpm run build" | ||
| }, | ||
| "dependencies": { | ||
| "@serverlessworkflow/i18n": "workspace:*", |
There was a problem hiding this comment.
This is a circular dependency
| "@serverlessworkflow/i18n": "workspace:*", |
| @@ -0,0 +1,29 @@ | |||
| { | |||
| "name": "@serverlessworkflow/i18n", | |||
There was a problem hiding this comment.
I don't think we are interested on publishing this package on npm registry as it is an internal package
| "name": "@serverlessworkflow/i18n", | |
| "name": "@serverlessworkflow/i18n", | |
| "private": true, |
| @@ -0,0 +1,29 @@ | |||
| { | |||
| "name": "@serverlessworkflow/i18n", | |||
| "version": "1.0.0", | |||
There was a problem hiding this comment.
| "version": "1.0.0", | |
| "version": "0.0.0", |
| "outDir": "dist", | ||
| "rootDir": "src", | ||
| "declaration": true, | ||
| "emitDeclarationOnly": false |
There was a problem hiding this comment.
This should not be necessary
| "emitDeclarationOnly": false |
| "build": "pnpm clean && tsc -p tsconfig.json", | ||
| "build:prod": "pnpm run build" |
There was a problem hiding this comment.
Please follow the repository style packages/serverless-workflow-diagram-editor/package.json:
| "build": "pnpm clean && tsc -p tsconfig.json", | |
| "build:prod": "pnpm run build" | |
| "build:dev": "pnpm clean && tsc -p tsconfig.json", | |
| "build:prod": "pnpm build:dev" |
| "types": "./dist/index.d.ts" | ||
| } | ||
| }, | ||
| "scripts": { |
There was a problem hiding this comment.
Please add the lint, format, test commands.
For the tests there is a guide here: https://react.i18next.com/misc/testing
| import { initReactI18next } from "react-i18next"; | ||
| import { resources, defaultNS } from "./resources"; | ||
|
|
||
| export async function setupI18n(instance: i18n) { |
There was a problem hiding this comment.
I would suggest to follow the official guide:
https://react.i18next.com/guides/quick-start#configure-i18next
It will result in less code and standard way
There was a problem hiding this comment.
These translations are related to packages/serverless-workflow-diagram-editor and should be there, if possible.
Wdyt @handreyrc ?
There was a problem hiding this comment.
I agree, translations associated with the editor package should be inside the folder packages/serverless-workflow-diagram-editor as they are specific to that package
| }, | ||
| "dependencies": { | ||
| "@serverlessworkflow/i18n": "workspace:*", | ||
| "i18next": "catalog:", |
There was a problem hiding this comment.
I don't think this is needed
| "i18next": "catalog:", |
There was a problem hiding this comment.
@lornakelly not in this PR and with low priority, maybe we should evaluate adding a script to verify the translation entries?
There was a problem hiding this comment.
Yes, I will add this to our excel and we can discuss what it would include eg missing keys between language files etc. Agree, this is a separate issue from this PR though
|
@kumaradityaraj Let me know when you have addressed @fantonangeli comments and I will review again, thanks! |
|
@lornakelly Handrey suggested something similar to implement like this - I18nDictionariesProvider So i am looking at it as this is more modular and easy to use |
|
@kumaradityaraj, maybe you can try with the official provider: https://react.i18next.com/latest/i18nextprovider ? |
Closes 15
Summary
This PR sets up internationalization (i18n) support for the project by introducing a shared
@serverlessworkflow/i18npackage and integrating it into the diagram editor.Changes
@serverlessworkflow/i18nto centralize i18n configuration.useTranslation()without additional setup.